lib: stop installing fetch if `no_browser_globals` is `true`#41969

Merged

nodejs-github-bot merged 1 commit intonodejs:masterfrom

RaisinTen:lib/stop-installing-fetch-if-no_browser_globals-is-true

Feb 16, 2022

Member

RaisinTen commented Feb 14, 2022

Fixes: #41816
Signed-off-by: Darshan Sen raisinten@gmail.com


          lib: stop installing fetch if no_browser_globals is true

6f7bae1

Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

Collaborator

nodejs-github-bot commented Feb 14, 2022

Review requested:

@nodejs/startup

nodejs-github-bot added the needs-ci label

RaisinTen mentioned this pull request

Fetch should not be installed if no_browser_globals==true #41816

Closed

Contributor

aduh95 commented Feb 14, 2022

I think if a user use --experimental-fetch, they do want the global fetch. If #41811 lands, then it would make sense to make the default value of --experimental-fetch equal to no_browser_globals, but until then I don't think we should restrict it more.

Member

targos commented Feb 14, 2022

@aduh95 I think no_browser_globals is here because there were issues with Electron and globals that exist both in Node.js and Chromium. If a user has --experimental-fetch in their NODE_OPTIONS, it could still be a problem.

benjamingr approved these changes

View reviewed changes

Contributor

aduh95 commented Feb 14, 2022

If we're doing this, the same check should be added for

node/lib/internal/bootstrap/pre_execution.js

Lines 169 to 172 in 849991c

    
           function setupWebCrypto() { 
        
             if (!getOptionValue('--experimental-global-webcrypto')) { 
        
               return; 
        
             }

Mesteery added a commit to Mesteery/node that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

15bcb4c

Refs: nodejs#41969 (comment)

Mesteery mentioned this pull request

lib: stop installing webcrypto if no_browser_globals is true #41971

Merged

lpinca approved these changes

View reviewed changes

Linkgoron approved these changes

View reviewed changes

panva approved these changes

View reviewed changes

Mesteery approved these changes

View reviewed changes

panva added the request-ci label

github-actions bot removed the request-ci label

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request

CI Reliability 2022-02-15 nodejs/reliability#203

Open

20 tasks

Collaborator

nodejs-github-bot commented Feb 15, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42570/

RaisinTen added the author ready label

RaisinTen mentioned this pull request

src: add fetch to bootstrap/browser.js #41958

Closed

targos approved these changes

View reviewed changes

github-actions bot mentioned this pull request

CI Reliability 2022-02-16 nodejs/reliability#204

Open

33 tasks

Mesteery added the commit-queue label

nodejs-github-bot removed the commit-queue label

nodejs-github-bot merged commit a82c1a1 into nodejs:master

Collaborator

nodejs-github-bot commented Feb 16, 2022

Landed in a82c1a1

github-actions bot mentioned this pull request

CI Reliability 2022-02-17 nodejs/reliability#205

Open

28 tasks

RaisinTen deleted the lib/stop-installing-fetch-if-no_browser_globals-is-true branch

February 17, 2022 04:49

nodejs-github-bot pushed a commit that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

fdab137

Refs: #41969 (comment)

PR-URL: #41971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>

github-actions bot mentioned this pull request

CI Reliability 2022-02-18 nodejs/reliability#206

Open

26 tasks

bengl pushed a commit to bengl/node that referenced this pull request


          lib: stop installing fetch if no_browser_globals is true

bd4e550

Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

bengl pushed a commit to bengl/node that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

2ab877f

Refs: nodejs#41969 (comment)

PR-URL: nodejs#41971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>

bengl pushed a commit to bengl/node that referenced this pull request


          lib: stop installing fetch if no_browser_globals is true

1f1198d

Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

bengl pushed a commit that referenced this pull request


          lib: stop installing fetch if no_browser_globals is true

6d8fb59

Fixes: #41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

bengl pushed a commit that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

9c783c3

Refs: #41969 (comment)

PR-URL: #41971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>

bengl mentioned this pull request

v17.6.0 release proposal #42072

Merged

bengl pushed a commit that referenced this pull request


          lib: stop installing fetch if no_browser_globals is true

8c912ab

Fixes: #41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

bengl pushed a commit that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

53e2e8c

Refs: #41969 (comment)

PR-URL: #41971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>

bengl pushed a commit that referenced this pull request


          lib: stop installing fetch if no_browser_globals is true

68ec8d5

Fixes: #41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

bengl pushed a commit that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

c3b0e72

Refs: #41969 (comment)

PR-URL: #41971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>

danielleadams added the backport-blocked-v16.x label

This comment was marked as resolved.

Sign in to view

aduh95 added dont-land-on-v12.x and removed backport-blocked-v16.x labels

Contributor

aduh95 commented Apr 20, 2022

no_browser_globals is not available on v16.x because #40532 never landed there.

danielleadams pushed a commit that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

Refs: #41969 (comment)

PR-URL: #41971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>

danielleadams pushed a commit that referenced this pull request


          lib: stop installing webcrypto if no_browser_globals is true

e80043f

Refs: #41969 (comment)

PR-URL: #41971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

panva panva approved these changes

benjamingr benjamingr approved these changes

lpinca lpinca approved these changes

targos targos approved these changes

+2 more reviewers

Linkgoron Linkgoron approved these changes

Mesteery Mesteery approved these changes

Labels

author ready needs-ci